Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Add generic SSO authentication support #1622

Closed
wants to merge 1 commit into from

Conversation

tribut
Copy link

@tribut tribut commented Aug 31, 2022

This adds SSO support using a trusted HTTP header from a reverse proxy.

There are a few rough edges before this can get merged, but given my lack of experience with nuxt, I would appreciate feedback on what I have right now.

How it works

A new setting is added to both front- and backend for the name of the trusted header.

The backend will unconditionally return a token for the user contained in this header.

The frontend will add the content of this header to the store using SSR (so its available on the client) and the new auth scheme will automatically request a token when this is present. If /login is opened and the header is not present, the user will be redirected to the external login page and conversely, they will be redirected to an external logout page after logging out.

That way, only /login and /api/auth/token have to be protected by the reverse proxy, everything else including public links will continue to work as always.

I've tested this with Apache (mod_auth_openidc) and authentik and so far it seems to work great.

Questions / Checklist

  • Is there some way to enable SSR in the devcontainer for easier testing? vite: { ssr: true } leads to errors (also without the changes in this PR).
  • Do we want to add SSO_TRUSTED_HEADER_ADMIN and SSO_TRUSTED_HEADER_GROUP (similar to LDAP improvements #1487)? Are others needed?
  • mounted/onMounted are not optimal for doing authentication, for example /login renders before the user is redirected to the app after login. Is there another option?
  • Add some tests
  • Write documentation, maybe provide a simple reverse proxy config

@hay-kot
Copy link
Collaborator

hay-kot commented Sep 10, 2022

Is there some way to enable SSR in the devcontainer for easier testing? vite: { ssr: true } leads to errors (also without the changes in this PR).

Not that I'm aware of. I've been struggling with this as well and have to build to the container to verify that things are working when working with SSR. I haven't dug too deep into this as I was hoping the Nuxt 3 Bridge would be ready for prime time, but it's doesn't look like it's ready for our use cases yet.

Do we want to add SSO_TRUSTED_HEADER_ADMIN and SSO_TRUSTED_HEADER_GROUP (similar to #1487)? Are others needed?

I'm assuming we'd need to, No? What's the alternative?

mounted/onMounted are not optimal for doing authentication, for example /login renders before the user is redirected to the app after login. Is there another option?

You probably want a plugin

https://nuxtjs.org/docs/directory-structure/plugins/

@hay-kot hay-kot marked this pull request as draft September 10, 2022 17:29
@PotentialIngenuity
Copy link

@tribut Any updates on this feature?

@aroberts
Copy link

I am also waiting for this feature- is there any chance it’s going to be merged?

@tribut
Copy link
Author

tribut commented Dec 27, 2022

Sorry everyone for letting you wait for so long. I'm going to add the missing headers and then send this off for final review. The plugin route didn't work and I've been using this branch for months now without any issues, so I think the way it's implemented is fine.

@aroberts
Copy link

Much appreciated! Do you publish an image with this feature? Even after merge it’ll be a while before this gets released I imagine, given the 1.0 release

@Zackptg5
Copy link

Zackptg5 commented Jan 1, 2023

Much appreciated! Do you publish an image with this feature? Even after merge it’ll be a while before this gets released I imagine, given the 1.0 release

Once it's merged you can just use the nightly branch or fork it and build it locally until next beta release

@hay-kot hay-kot closed this Jan 8, 2023
@knrdl
Copy link

knrdl commented Jan 14, 2023

Hi there, is there any chance to merge this? The feature is an important requirement for me to use mealie.

@aroberts
Copy link

I also depend on this feature- @hay-kot can you comment on why this was closed, and whether or not you’re planning to include this feature down the road?

@hay-kot
Copy link
Collaborator

hay-kot commented Jan 15, 2023

This PR was closed after being a draft for nearly 5 months. It was not merged because

  • the author stated it was incomplete
  • there is no documentation on how to use this feature.
  • there are no tests included in the PR.

I have no plans on doing worked related to this, it's been asked again and again here and on discord and my stance is still the same.

I have no use for this feature, and no clear way to build or test it. I'm open to community contributions to add it as a feature, but given the way LDAP has worked out since its addition, my standards for PRs altering authentication are going to be higher than l others.


I do really appreciate the work the Author did here and I know people would love this to be added to the application, but life happens and that is 100% okay. Hopefully someone else can take this over the last few hurdles so it can be added to Mealie.

Cheers!

@knrdl
Copy link

knrdl commented Jan 15, 2023

Thanks for the fast reply. I totally see your point. I will look into it and maybe open a new PR!

@aroberts
Copy link

@hay-kot totally defensible stance, and I am grateful you’re leaving the door open for the community to bring this home.

I am also attempting to look into this a bit, but I have no experience with this framework (so @knrdl please also dig in!). In an above comment you recommended a plug-in instead of using onMount- why a plug-in and not a middleware?

@tribut
Copy link
Author

tribut commented Jan 15, 2023

Still planning on finishing this, just very little time. Sorry I can't give a proper timeframe.

@aroberts
Copy link

@tribut I totally hear you on the time front, no problem, and I appreciate your continued effort on this. It seems like there's definitely an audience ready to consume this feature!

@knrdl knrdl mentioned this pull request Jan 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants